-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove nulls from UncheckedProc #885 #1029
Remove nulls from UncheckedProc #885 #1029
Conversation
Job #1029 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #1029 +/- ##
============================================
+ Coverage 87.4% 87.47% +0.06%
Complexity 1515 1515
============================================
Files 266 266
Lines 3884 3887 +3
Branches 212 212
============================================
+ Hits 3395 3400 +5
+ Misses 440 438 -2
Partials 49 49
Continue to review full report at Codecov.
|
This pull request #1029 is assigned to @fabriciofx/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victornoel Done. Please, check out.
@@ -53,7 +52,11 @@ public UncheckedProc(final Proc<X> prc) { | |||
|
|||
@Override | |||
public void exec(final X input) { | |||
new UncheckedFunc<>(new FuncOf<>(this.proc, null)).apply(input); | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victornoel Please, can you add some tests here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabriciofx there already are tests for this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victornoel Right. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victornoel you can use CheckedScalar
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llorllale if I use CheckedScalar
then I will need to specify an object to be returned, and then I will put null
:) FuncOf
does use one of the Scalar
class for example
@victornoel This PR seems fine. Thanks! @llorllale Can you merge it now? |
@rultor merge |
@fabriciofx Thanks for your request. @llorllale Please confirm this. |
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale @victornoel Oops, I failed. You can see the full log here (spent 3min)
|
@llorllale I suppose the above is not meant to happen? |
@victornoel no... have you tried running gitlint on your local? |
@llorllale the error above is not very clear, but on my local, I can see the actual error and it says I have to start the commit message with the issue number.
Edit: I went too fast, actually the issue number must be inside parenthesis :) |
748e123
to
b1983cf
Compare
@llorllale I fixed the commit ;) |
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale @victornoel Oops, I failed. You can see the full log here (spent 3min)
|
b1983cf
to
9502abf
Compare
@llorllale I don't understand what is the problem, it is as if there was some utf8 characters somewhere, but I can't reproduce on my machine… also it seems that travis does not exhibit the problem, only rultor… |
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale Done! FYI, the full log is here (took me 14min) |
The job #1029 is now out of scope |
Order was finished: +15 point(s) just awarded to @fabriciofx/z |
Payment to |
This is for #885